Skip to content

feat(history): add typed inherent methods and JSON filter to SqliteBackedHistory#1112

Merged
kronberger-droid merged 12 commits into
nushell:mainfrom
eitsupi:feat/typed-history-extra-info
Jul 22, 2026
Merged

feat(history): add typed inherent methods and JSON filter to SqliteBackedHistory#1112
kronberger-droid merged 12 commits into
nushell:mainfrom
eitsupi:feat/typed-history-extra-info

Conversation

@eitsupi

@eitsupi eitsupi commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Three typed inherent methods are added to SqliteBackedHistory (save_with_extra, load_with_extra, search_with_extra) for round-tripping more_info without type erasure. History::save now delegates to save_with_extra, so both paths share the same implementation. A new JsonFilterValue enum and SearchFilter::more_info_json field enable type-precise filtering by JSON path at the SQL level. The History trait is unchanged.

Motivation

#1011 made HistoryItem generic over ExtraInfo and exposed HistoryItemExtraInfo publicly. However, the History trait still uses the type-erased HistoryItem<IgnoreAllExtraInfo>, so any more_info written through the trait is silently discarded on the next read. The generics are in place, but there is no usable end-to-end path to preserve custom metadata.

Downstream crates that want to persist typed metadata in more_info necessarily hold SqliteBackedHistory directly, since FileBackedHistory has no more_info support at all. Typed inherent methods on SqliteBackedHistory model this capability boundary precisely, without changing the History trait.

@eitsupi
eitsupi force-pushed the feat/typed-history-extra-info branch from 79faf98 to a04be41 Compare June 28, 2026 10:16
eitsupi and others added 6 commits June 28, 2026 10:17
Add `save_with_extra`, `load_with_extra`, and `search_with_extra` as
typed inherent methods on `SqliteBackedHistory`. These methods allow
downstream crates to round-trip custom `more_info` types through the
SQLite backend without going through the `History` trait.

Also add `more_info_json` to `SearchFilter` for SQL-level JSON filtering
using SQLite's `json_extract()`. This avoids fetching all rows and
filtering in Rust when only a subset matching a JSON path condition is
needed. Boolean JSON values compare as integers (`"1"` = true, `"0"` =
false) because SQLite's `json_extract` maps JSON booleans to integers.

The `History` trait itself remains unchanged (non-breaking).

Motivation: downstream crates that hold `SqliteBackedHistory` directly
(rather than `Box<dyn History>`) can now store and query typed metadata
alongside history entries. Crates using `FileBackedHistory` or
`Box<dyn History>` are unaffected.
…r booleans

History::count() and History::search() now clear more_info_json before
calling construct_query, matching the documented contract that non-typed
trait methods do not evaluate JSON filters.

For more_info_json filter values of "true" or "false", use SQLite's
json_type() instead of CAST(json_extract(...) AS TEXT) to match JSON
boolean values exactly. This prevents false matches against integer 1/0
and string "true"/"false". Other filter values continue to use CAST.
…alue enum

Add `JsonFilterValue` enum with `Null`, `Bool`, `Integer`, `Real`, and `Text`
variants so that `SearchFilter::more_info_json` filters are type-precise.
Each variant generates SQL using `json_type()` to prevent collisions between
JSON booleans, integers, and strings (e.g. `Bool(true)` no longer matches
integer `1` or string `"true"`).

Export `JsonFilterValue` from the crate root. Add collision tests covering
every scalar type combination.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add json_filter_value_real_vs_integer to verify Real(f) does not collide
with Integer values sharing the same numeric representation, and
json_filter_value_null_vs_missing_path to verify Null matches JSON null
at a path but not SQL-NULL more_info or an absent path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
json_filter_value_real_vs_integer: add an integer row at $.score to verify
Real(1.0) excludes it (json_type 'integer' != 'real') while Integer(1)
correctly matches it but not the real rows.

json_filter_value_null_vs_missing_path: add a row whose non-null JSON
has no $.val key (WithoutVal struct) to verify JsonFilterValue::Null
excludes it alongside the SQL-NULL more_info row.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eitsupi
eitsupi force-pushed the feat/typed-history-extra-info branch from a04be41 to 6f3efef Compare June 28, 2026 10:17
eitsupi and others added 2 commits June 28, 2026 10:25
Keep BoxedNamedParams as &'static str keys (unchanged from upstream) to
avoid adding .to_string() to every existing params.push call. Dynamic
JSON filter parameters (":json_path_N" etc.) are collected in a separate
OwnedNamedParams vec and merged at the end of construct_query.

Also remove the spurious Vec<&str> type annotation on `wheres`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
History::save now delegates to save_with_extra rather than a private
save_impl, removing the unnecessary indirection layer.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@eitsupi
eitsupi marked this pull request as ready for review June 28, 2026 10:33
@fdncred

fdncred commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

cool. i remember adding more_info thinking someday someone would use it. what are you using it for?

@eitsupi

eitsupi commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

what are you using it for?

My priorities were distinguishing between special meta-commands available within the REPL and recording the REPL's output.
https://github.com/eitsupi/arf/blob/d95cd3f875fd495909034050e3b1884e9291bde1/crates/arf-console/src/history/search.rs#L96-L100

Furthermore, I imagine this would be useful for this tool with REPL that can connect to various databases, such as for distinguishing between different databases.
https://github.com/columnar-tech/databow

@fdncred

fdncred commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

cool. thanks for sharing.

@eitsupi

eitsupi commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

My first use case: eitsupi/arf#236

@fdncred

fdncred commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@kronberger-droid I'd probably wait on this one too until after the release unless you feel strongly about landing it now.

@kronberger-droid

kronberger-droid commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Yeah agreed.

@kronberger-droid

Copy link
Copy Markdown
Collaborator

Went through it now.

Two things before we land it:

  1. There is no update_with_extra.
    The trait update is a load+save through the erased path, so load drops more_info and save writes it back as null.
    update_last_command_context goes through there, which means the normal post command bookkeeping wipes whatever save_with_extra wrote.

  2. JsonFilterValue should be #[non_exhaustive].
    The docs say arrays and objects are intentionally unsupported, which sounds like a later addition.

Also the trait search/count silently null out more_info_json instead of erroring, so a &dyn History caller quietly gets broader results.

Happy to land once the update path (1) is sorted.
The others are optional i guess.
Thanks!

eitsupi added 2 commits July 21, 2026 22:54
History::update() went through the type-erased load/save path, which
always deserializes more_info as IgnoreAllExtraInfo and re-serializes
it as null, silently wiping out data written via save_with_extra. Any
consumer that calls Reedline::update_last_command_context for normal
post-command bookkeeping would clobber typed more_info on every run.

Change the trait update() to update every column except more_info,
and add an update_with_extra<E>() inherent method for callers that
need to read/modify typed more_info atomically.

Also mark JsonFilterValue as non_exhaustive since array/object
support is a plausible future addition.
@eitsupi

eitsupi commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for your review; I believe I have addressed the points you raised.

The fact that search/count does not take more_info into account is intentional, and I believe it is reasonable for the History trait methods to ignore more_info since it only works with the SQLite backend.

eitsupi added 2 commits July 22, 2026 12:01
update_with_extra claimed atomicity but performed the load and save as
separate statements outside a transaction, so a concurrent writer could
interleave between them (lost updates, or resurrecting a row deleted in
between). Share the query logic with save_with_extra/load_with_extra via
connection-generic helpers and run both steps inside a single immediate
transaction.
Add update_with_extra_blocks_concurrent_writer, which uses two file-backed
connections to the same database to confirm that update_with_extra's
immediate transaction actually blocks a concurrent writer (previously there
was no test that would fail without the transaction).
@eitsupi

eitsupi commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

I found a bug related to transactions, so I fixed it in the last two commits and added tests.
I believe it's merge-ready.

@kronberger-droid

Copy link
Copy Markdown
Collaborator

I will have Claude run over it once, then I would also merge.
@fdncred fine for you too?

@fdncred

fdncred commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

ya, i'm good with this. we can land this when you're ready.

@kronberger-droid

Copy link
Copy Markdown
Collaborator

Fine from my side, too.
After we merge this i will bump reedline on nushell to include the last to changes.

@fdncred

fdncred commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

ok, have at it

@kronberger-droid
kronberger-droid merged commit 83c17a2 into nushell:main Jul 22, 2026
7 checks passed
@eitsupi
eitsupi deleted the feat/typed-history-extra-info branch July 22, 2026 14:15
fdncred pushed a commit to nushell/nushell that referenced this pull request Jul 22, 2026
<!--
Thanks for contributing to Nushell!

Before submitting, please read the contributing guide:
https://github.com/nushell/nushell/blob/main/CONTRIBUTING.md

This template helps reviewers understand your changes and allows us to
generate high-quality release notes.
-->

## Description
<!--
Explain what this PR does and why.

This section is intentionally flexible:
- Describe the problem
- Explain your approach
- Include technical details if relevant

Good examples:
- "In this PR, I fixed..."
- "In this PR, I added support for..."
- "This change improves X by..."

Write as much or as little as needed for reviewers to understand your
changes.
-->

Includes nushell/reedline#1128, which switches
the `Prompt` trait to `nu_ansi_term::Color` exclusively, and
nushell/reedline#1112, which adds typed inherent
methods and a JSON filter to `SqliteBackedHistory`.

Despite nushell/reedline#1128 being a breaking
change, nushell needs no source changes. `NushellPrompt` only ever used
the defaulted `get_*_color` methods, so the new return type has no
effect here.

## User-facing changes (Release notes)
<!--
This section is used (mostly as-is) for https://www.nushell.sh/blog/

Describe how Nushell behavior changes from a user's perspective.
Do NOT describe internal Rust changes here.

Write in a release note style, for example:
- "Added support for..."
- "Fixed an issue where..."
- "Nushell now supports..."
- "Improved performance of..."

If your changes do NOT affect users (internal refactors, cleanup, etc.),
just write:
- "n/a"
- "nan"
- or similar

This tells us the change should not appear in the changelog.

Tips:
- Focus on observable behavior
- Include examples if helpful
- Keep it concise

You can:
- Write a short paragraph (will appear as a bullet point), OR
- Use headings (###) if your change needs more structure

Avoid writing things like:
- "In this PR, I refactored..."
- "This updates internal code..."

You may leave this blank until the PR is ready.
-->

## Additional notes
<!--
Optional.

Examples:
- fixes #123
- closes #456
- related #789

Anything else reviewers should know.
Remove this section if not needed.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants